Parallel fixes - #2585
Open
wangcj05 wants to merge 12 commits into
Open
Conversation
…afe node discovery (B1,B2,B6,B10-slurm) - Initialize self.__runSbatch = False in __init__ so remoteRunCommand() no longer raises AttributeError when <runSbatch> is absent (B1). - Declare <nodefile>/<nodefileenv> in the input specification and handle all option names case-insensitively; <noprecommand> previously never matched the camelCase comparison in handleInput (B2). Unknown options now warn. - <nodefileenv> validates the environment variable exists (B10). - Replace unchecked 'os.system(srun ... > file)' with a checked subprocess call, plus a 'scontrol show hostnames' + SLURM_TASKS_PER_NODE fallback in the new ClusterUtils module; empty node files now raise a clear error (B6). - Close node file handles via context managers; replace debug print() with message-handler calls; fix stale 'Not in PBS' comment.
kwargs['INDEX1'] was computed as str(i+i) (i.e. 2*i) instead of the intended 1-based batch-slot index str(i+1). Any code-interface command using the %INDEX1% placeholder received 0,2,4,... for slots 0,1,2,..., silently selecting the wrong node files / bindings for batch slots >= 1.
- The Dask branch of __shutdownParallel guarded on rayInstanciatedOutside instead of daskInstanciatedOutside; with an externally provided scheduler RAVEN could tear down resources it does not own. - When RAVEN owns the cluster, call Client.shutdown() so the scheduler retires ALL workers (including the ssh-launched remote 'dask worker' processes, which previously had no teardown path and were left as zombies), then terminate the tracked head-node worker and scheduler subprocesses. - Track the head-node dask worker Popen (previously a dropped local variable). - Close ray_head.ip and dask worker log file handles via context managers. - Guard Ray address_info debug prints with .get(); 'redis_address' was removed in Ray 2.x and raised KeyError under verbose debugging.
RayRunner.kill() and DaskRunner.kill() only dropped the local ObjectRef / Future, so 'killed' remote tasks kept executing and consuming cluster cores (affecting optimizer early stopping, terminateJobs and terminateAll). Now kill() issues ray.cancel(force=True, recursive=True) / Future.cancel() before releasing the reference, with a warning if cancellation fails.
…; propagate tracebacks (B7, quick-ref #7) - InternalRunner gains an explicit tri-state runSucceeded flag and a failureInfo string (formatted traceback / RayTaskError text), exposed via getFailureInfo(). getEvaluation() now returns Error() based on the explicit flag; the 'None return == failure' sentinel remains only as a legacy fallback when no outcome was recorded. - SharedMemoryRunner: the bare lambda thread target is replaced by a wrapper that catches exceptions, records the traceback and the outcome; functions that legitimately return None are no longer marked failed. - RayRunner/DaskRunner: success and failure are recorded explicitly on ray.get()/Future.result(); failure text is kept. DaskRunner no longer re-raises inside _collectRunnerResponse, so getEvaluation() cannot blow up the collecting thread (getReturnCode retains its failure semantics). - JobHandler.__checkAndRemoveFinished logs the failure details and stores them under 'failureInfo' in the failed-job metadata.
PyThreadState_SetAsyncExc is ignored while a thread is blocked in C extensions or system calls, so the previous 'while alive: sleep; kill()' loop could spin forever. kill() now retries for a bounded 10 s window, then warns and abandons the (daemon) thread, marking the run as failed explicitly.
…s, quote variables (B9) - start_remote_servers.sh: '--help' used 'return' which is invalid in an executed script (now 'exit 0'); error paths exit non-zero; START_OUTPUT was referenced by tee but never defined (now <output>_start.log); the parsed --num-gpus value was silently dropped and is now forwarded to start_ray.sh; variable expansions are quoted. - start_ray.sh: new NUM_GPUS positional argument, passed to 'ray start --num-gpus' only when non-negative; optional bash profile; quoted variables. - start_dask.sh: the remote bash profile is now optional (previously 'source ""' failed when unset); 'cd' is checked; quoted variables.
…le (B10) PBS and MPI-legacy modes read os.environ[<nodefileenv>] directly, producing a raw KeyError traceback when the variable is undefined. Both now raise a RAVEN IOError naming the missing variable.
…, add unit tests (quick-ref #8) - New ClusterMode.ClusterSimulationMode base class holds the previously triplicated modifyInfo logic: node-file reading, batch-size clamping, per-batch node_%INDEX% file splitting, mpiexec precommand assembly and thread postcommand handling. Slurm and PBS modes are now thin subclasses that only perform scheduler-specific node discovery and submission (the MPI legacy mode is intentionally left untouched). - ClusterUtils gains pure, framework-independent functions (readNodeFile, computeBatchSize, writeNodeSubFiles, buildMPIPrecommand, sanitizeJobName) used by the base class. - Job-name validation is unified via ClusterUtils.sanitizeJobName: both modes now accept alphanumerics, '_' and '-' (the Slurm mode previously rejected '-' while PBS allowed it), PBS keeps its 15-character truncation, and both raise RAVEN errors instead of bare IOError/print. - Empty PBS node files now raise a clear error (previously silently produced batchSize=1); PBS node-file handles are context-managed via readNodeFile. - New standalone unit tests (21 cases) in CustomModes/tests/, runnable without a built RAVEN: python3 ravenframework/CustomModes/tests/test_cluster_utils.py
New optional <useSrun/> mode option: run precommands become 'srun --overlap --exact -n <NumMPI> [MPIParam...]' instead of 'mpiexec <NodeParameter> <nodefile> -n <NumMPI>'. Slurm assigns per-step resources itself, so the per-batch node_%INDEX% files are skipped entirely, removing the hostfile-flag dependence on the MPI flavor (MPICH -f vs OpenMPI --hostfile) and working with all major MPI stacks via PMI/PMIx. Node discovery is kept so runInfo['Nodes'] remains available for internal-parallel cluster bring-up. Extra <MPIParam> entries are passed to srun (e.g. --mpi=pmix). Includes unit tests for the precommand construction.
…ref #10) New <daskJobqueue> RunInfo element (used with <internalParallel>True </internalParallel> and <parallelMethod>dask</parallelMethod>): the element text selects the scheduler ('slurm' or 'pbs') and the XML attributes are passed to the dask_jobqueue SLURMCluster/PBSCluster constructor. Example: <daskJobqueue memory="4GB" queue="short" account="proj1">slurm</daskJobqueue> Dask workers are then submitted AS scheduler jobs by dask_jobqueue, removing the inter-node-ssh requirement and the hand-rolled bring-up scripts for this path, with clean teardown (client + cluster close cancels the worker jobs). 'memory' is required (dask_jobqueue mandates it); 'cores' defaults to numProcByRun, 'jobs' (number of worker scheduler jobs) defaults to batchSize, 'walltime' defaults to <expectedTime>; other attributes pass through verbatim. Fails with a clear error when dask_jobqueue is not installed. Kwargs assembly lives in ClusterUtils.assembleDaskJobqueueKwargs (pure, covered by unit tests). The cluster object is excluded from pickling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
What are the significant changes in functionality due to this change request?
Contents (one commit per patch, messages included)
__runSbatchcrash (B1),<noprecommand>/<nodefile>input handling (B2), checkedsrunnode discovery withscontrolfallback (B6),<nodefileenv>validation%INDEX1%placeholder was2*i, nowi+1(B3)RayRunner.kill/DaskRunner.killactually cancel the remote task (B5)runSucceededflag: legitNonereturns aren't failures; failure tracebacks propagated to logs and failed-job metadata (B7, #7)--helpexit,START_OUTPUT,--num-gpusforwarding, quoting, optional bash profile (B9)<nodefileenv>env var (B10)ClusterSimulationModebase +ClusterUtilsmodule; Slurm/PBS refactored; unified job-name rules; 21+ unit tests (#8)<useSrun/>native launch mode (#9)<daskJobqueue>RunInfo option using dask-jobqueue SLURMCluster/PBSCluster (#10)For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>to True.raven/tests/framework/user_guideandraven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.